Skip to content

fix(MessagesList) - performance, search fixes and minor refactoring#9897

Merged
Antreesy merged 6 commits intomasterfrom
fix/noid/solve-type-errors
Jul 19, 2023
Merged

fix(MessagesList) - performance, search fixes and minor refactoring#9897
Antreesy merged 6 commits intomasterfrom
fix/noid/solve-type-errors

Conversation

@Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Jul 4, 2023 •
edited by ShGKme
Loading

☑️ Resolves

Splitted by commits:

  • Limit lookForNewMessages request with default fetch limit (100 messages)
  • Fix store regression which leads to unnecessary fetch requests

🖼️ Screenshots: see #9897 (comment)

  • Adjust scroll position when loading new / old messages
    • now list remains on the same place after re-render and don't jump up-down because of loader insert into DOM

🖼️ Screenshots

Before After
image image
  • Fix focusing and highlighting of messages, targeted from replies and unified search

🖼️ Screenshots

Before After
image image
  • Fix highlighting of focused message
    • now it's done with Vue approach - refs + public methods

🚧 Tasks

  • Code review
  • Manual testing

🏁 Checklist

@Antreesy Antreesy added this to the 💜 Next Major (28) milestone Jul 4, 2023
@Antreesy Antreesy requested review from DorraJaouad and ShGKme July 4, 2023 15:37
@Antreesy Antreesy self-assigned this Jul 4, 2023
@Antreesy
Copy link
Contributor Author

Antreesy commented Jul 4, 2023

/backport to stable27

@Antreesy Antreesy requested a review from marcoambrosini July 4, 2023 15:39
nickvergessen
nickvergessen reviewed Jul 5, 2023
View reviewed changes
@Antreesy Antreesy marked this pull request as draft July 5, 2023 09:59
@Antreesy
Copy link
Contributor Author

Antreesy commented Jul 5, 2023

Converted to draft to solve #6046 and backport together, because of related changes (message focus)

@Antreesy Antreesy force-pushed the fix/noid/solve-type-errors branch 2 times, most recently from f9f2a85 to c994ae6 Compare July 6, 2023 13:34
@Antreesy Antreesy changed the title fix(TypeError) - fix "Cannot read properties of undefined" for message components fix(MessagesList) - performance, search fixes and minor refactoring Jul 6, 2023
@Antreesy Antreesy force-pushed the fix/noid/solve-type-errors branch 2 times, most recently from e96978e to 87f1d23 Compare July 6, 2023 14:44
@Antreesy Antreesy marked this pull request as ready for review July 6, 2023 15:05
@Antreesy Antreesy mentioned this pull request Jul 7, 2023
Merged
8 tasks
@Antreesy Antreesy force-pushed the fix/noid/solve-type-errors branch from 87f1d23 to c9c1fc9 Compare July 13, 2023 17:41
ShGKme
ShGKme requested changes Jul 17, 2023
View reviewed changes
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested at least to search behavior changes.

This PR of solve-type-errors branch includes:

  • 3 (probably 4) different bug fixes (without bugs' descriptions)
  • 3 visual changes (although there is "🖼 Screenshots No visual changes" in the description)
  • 1 UX change (search behavior)
  • 1 performance update (according to the title)
  • Refactorings
  • And other minor changes

For me, it is hard to review and keep discussions about so many (sometimes not obvious and not small) changes in one PR. I'd prefer to have them in separated PRs in future.

Separating the changes also makes it easier to backport them to different stables when necessary.

@nextcloud nextcloud deleted a comment from Antreesy Jul 17, 2023
@nickvergessen
Copy link
Member

nickvergessen commented Jul 17, 2023

JFYI: I canceled the backport request to 26.

Also I'm very hesitant to backport this at all.

@Antreesy Antreesy mentioned this pull request Jul 17, 2023
Closed
@Antreesy
resolve Uncaught Typerror and focus-trap warn for reactions
795a36f 
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy Antreesy force-pushed the fix/noid/solve-type-errors branch from c9c1fc9 to c8d44a4 Compare July 18, 2023 12:32
@Antreesy
Copy link
Contributor Author

Antreesy commented Jul 18, 2023

Rebased onto master, fixups are squashed into original commits to keep PR clean
@ShGKme sorry for the mess 👀

@Antreesy Antreesy requested a review from ShGKme July 18, 2023 12:56
ShGKme
ShGKme reviewed Jul 18, 2023
View reviewed changes
Antreesy added 5 commits July 19, 2023 11:12
@Antreesy
limit fetch requests for messages evenly
ad58c72 
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy
soft update of message groups to prevent unnecessary re-rendering
6d0dc44 
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy
compensate increased list height for smooth chat scrolling
3c603fe 
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy
get context for searched messages if they are not presented in store yet
92a6332 
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy
replace messages highlighting method with Vue approach
7f765cc 
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy Antreesy force-pushed the fix/noid/solve-type-errors branch from c8d44a4 to 7f765cc Compare July 19, 2023 09:21
ShGKme
ShGKme reviewed Jul 19, 2023
View reviewed changes
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good from the code, but I haven't tested it yet after updates

ShGKme
ShGKme approved these changes Jul 19, 2023
View reviewed changes
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested, looks like it works fine 😃

@Antreesy Antreesy merged commit ac00eda into master Jul 19, 2023
@Antreesy Antreesy deleted the fix/noid/solve-type-errors branch July 19, 2023 13:56
@ShGKme
Copy link
Contributor

ShGKme commented Jul 19, 2023 •
edited
Loading

For future. Generate and send 1000 messages with text 1..1000 from the console

const START = 1
const MESSAGES_PER_SECOND = 5
const END = 1000

let i = START

const input = document.querySelector('[contenteditable]')
const getSend = () => document.querySelector('[aria-label="Send message"]')

const id = setInterval(async () => {
  input.innerHTML = i++;
  input.dispatchEvent(new InputEvent('input'));
  await Promise.resolve()
  getSend().click()
  if (i > END) {
    clearInterval(id)
  }
}, ~~(1000 / MESSAGES_PER_SECOND))

@Antreesy
Copy link
Contributor Author

Antreesy commented Jul 19, 2023 •
edited
Loading

So, this PR should be backported as well to resolve conflicts (cc @nickvergessen):

@Antreesy Antreesy mentioned this pull request Jul 24, 2023
Merged
6 tasks
@Antreesy
Copy link
Contributor Author

Antreesy commented Aug 7, 2023

/backport to stable27

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Aug 7, 2023
Merged
@Antreesy
Copy link
Contributor Author

Antreesy commented Aug 7, 2023

follow-up: although groups are now compared and updated softly during fetching, Message components inside are still re-rendered (probably some store mutations are causing it)

@nextcloud nextcloud deleted a comment from backportbot-nextcloud bot Aug 7, 2023
@Antreesy Antreesy mentioned this pull request Sep 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickvergessen nickvergessen nickvergessen left review comments

@ShGKme ShGKme ShGKme approved these changes

@DorraJaouad DorraJaouad Awaiting requested review from DorraJaouad

@marcoambrosini marcoambrosini Awaiting requested review from marcoambrosini

Assignees

@Antreesy Antreesy

Labels

bug feature: frontend 🖌️ "Web UI" client

Projects

None yet

Milestone

v18.0.0-beta.1

Development

Successfully merging this pull request may close these issues.

Duplicate date header for the same date Messages chat polling freezes the browser

3 participants

@Antreesy @nickvergessen @ShGKme